London | 26-ITP-Sep | Hanna Bohlin | Sprint 1 | formatAs12HourClock by habohlin · Pull Request #1628 · CodeYourFuture/Module-Structuring-and-Testing-Data

habohlin · 2026-09-28T16:13:40Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Task code

CYF-1197

Changelist

I wrote tests for the function for as many edge cases I could think of, and corrected the function to pass the tests when the tests failed.

illicitonion

This works and generally looks good, but I left a few comments to think about :)

illicitonion · 2026-09-28T17:22:56Z

+test("can format afternoon time with minutes other than 00", () =>
+  assert.equal(formatAs12HourClock("15:45"), "03:45 pm"));
+
+test("can format morning time with complex minutes", () =>
+  assert.equal(formatAs12HourClock("08:25"), "08:25 am"));
+
+test("can format early noon complex minutes", () =>
+  assert.equal(formatAs12HourClock("12:17"), "12:17 pm"));
+
+test("can format between midnight and 1 am", () =>


These are really good tests but you're using inconsistent terminology here - sometimes you're saying "minutes other than 00" and other times "complex minutes". By using different terms it makes me as a reader wonder whether they have different meanings. If you mean the same thing, I'd recommend using the same term.

Consistent now, thanks.

illicitonion · 2026-09-28T17:23:27Z

-test("correctly convert time after 12:00", function(){
-    assert.equal(formatAs12HourClock("23:00"), "11:00 pm");
-});
+test("correctly convert time after 12:00", () =>


This looks like a pretty thorough set of tests - well done!

illicitonion · 2026-09-28T17:24:59Z

-
  const hours = Number(time.slice(0, 2));

  if (hours > 12) {


I notice that in several of your branches you're doing the same thing - writing time.slice(-2) - if you had to change that for some reason, you'd need to change both copies. Can you think how to avoid this duplication?

Sorted: minutes is worked out once on line 3.

illicitonion · 2026-09-28T17:28:08Z

+  } else if (hours === 12) {
+    return `${time} pm`;
+  } else if (hours === 0) {
+    return `12:${time.slice(-2)} am`;


I've noticed that all of these branches have something in common - at its core, all of them are calculating an "hours", calculating a "minutes", and appending an "am" or "pm"

Often it can be useful to make clear in code what things are the same and what things are different. Can you think how you may structure this code so that you always just return ${hours}:${minutes} am/pm, but make clear with your if statement how you're differently computing those things?

Much clearer now, with one return and the ifs only deciding the hour and the period.

…. Only return once at the end

abdishakoor-dev

The refactor reads well now: minutes is worked out once, the two if statements only decide the hour and the period, and there is a single return. I checked it against midnight, noon and the minutes either side of both, and every one comes out right. Two things before I can mark it Complete:

timeConverter.test.js: the exact boundaries aren't tested yet. See the comment on line 23.
timeConverter.js fails Prettier again since the last commit (spaces before {, around : and -, and a missing semicolon on line 22). You formatted it once already in an earlier commit, so turning on format on save will stop this coming back: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md

Add the Needs Review label again once you've pushed.

abdishakoor-dev · 2026-10-04T08:48:09Z

+test("can format early noon with minutes other than 00", () =>
+  assert.equal(formatAs12HourClock("12:17"), "12:17 pm"));
+
+test("can format time between midnight and 1 am", () =>


00:15 checks the midnight hour, but not midnight itself. Which input is the very first minute of the day, which are the last minute before noon and the last minute before midnight, and which is the first time your hours >= 13 branch handles? Those are the places an if on hours is most likely to go wrong, so each is worth its own test.

All four are covered now on lines 26-36, thanks.

abdishakoor-dev · 2026-10-04T08:48:09Z

+  if (hours >= 13){
+    hourString = hours - 12 < 10 ? `0${hours - 12}` : `${hours -12}`;
+  } else if (hours === 0){
+    hourString = `${hours + 12}`;


Optional: when hours is 0, what is hours + 12 always going to be? Would writing that value directly make this branch easier to read?

abdishakoor-dev

Both points from last time are done: the four boundary tests (00:00, 11:59, 23:59, 13:00) are in and pass, and both files are Prettier-clean now. I ran the function on midnight, noon and the minute either side of each, and every one is right. Writing each new test in its own commit with whether it passed made the history easy to follow.

Marking this Complete.

habohlin added 10 commits September 28, 2026 11:44

Add test to convert noon time, "12:00" (which fails)

58741a8

Refactor the previous test funcs to use arrow funcs

53b309b

Add else clause to function to properly format noon time

6e1e485

Add test for time with minutes other than 00, (it fails)

c0c27ea

Add possible preceding 0 and exact minutes to times after noon

99591de

Add test for morning time complex mins (it passes)

891851b

Add test for early noon complex minutes (it passes)

8c42ec0

Add test for time between midnight and 1 am (it fails)

72e8b26

Add else if for when hour = 0, now the final test passes

ebfc759

Reformatted with prettier

7a51317

This comment has been minimized.

Sign in to view

habohlin added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 28, 2026

This comment has been minimized.

Sign in to view

github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 28, 2026

habohlin added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 28, 2026

illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 28, 2026

illicitonion reviewed Sep 28, 2026

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Sep 28, 2026

habohlin added 2 commits September 28, 2026 21:52

Rephrase test messages so they are more consistent

2c29789

Refactor the code to have separate if statements for hours and period…

e231337

…. Only return once at the end

habohlin added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 3, 2026

abdishakoor-dev reviewed Oct 4, 2026

View reviewed changes

abdishakoor-dev added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 4, 2026

habohlin added 3 commits October 4, 2026 17:10

Add test for exactly midnight (passes)

6f21d29

Add test for last minute before noon (it passes)

461f54b

Add test for last minute before midnight (it passes)

1ddd306

habohlin added 2 commits October 4, 2026 17:15

Add test for first time hour >= 13 (it passes)

04df6ca

Format with prettier

babce7a

habohlin added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 4, 2026

abdishakoor-dev reviewed Oct 5, 2026

View reviewed changes

abdishakoor-dev added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 5, 2026

cjyuan mentioned this pull request Oct 5, 2026

Manchester | 26-ITP-Sep | Salah Alsabhi | Sprint 1 | format clock edge cases #1642

Open

4 tasks done

Uh oh!

Conversation

habohlin commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Task code

Changelist

Uh oh!

This comment has been minimized.

This comment has been minimized.

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdishakoor-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdishakoor-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

habohlin commented Sep 28, 2026 •

edited

Loading